-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: allow toggler to be disabled #601
base: master
Are you sure you want to change the base?
feat: allow toggler to be disabled #601
Conversation
d31dd6a
to
c0946f2
Compare
c0946f2
to
856cc5b
Compare
@@ -7,7 +7,7 @@ use iced_core::text; | |||
pub fn toggler<'a, Message, Theme: iced_widget::toggler::StyleSheet, Renderer>( | |||
label: impl Into<Option<String>>, | |||
is_checked: bool, | |||
f: impl Fn(bool) -> Message + 'a, | |||
f: Option<impl Fn(bool) -> Message + 'a>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is optional, it should be moved to a method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense, especially as this will help not making breaking change to the API. Just to confirm, are you happy with having two functions, like:
toggler(label, is_checked, closure): Toggler
(existing)toggler(label, is_checked): Toggler
And add a method (and maybe a *_maybe
variation):
Toggler::on_change(impl Fn(bool) -> Message + 'a)
Is that what you had in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine to make breaking changes where necessary. There should be a method if we make the message optional. But we may want to wait for the iced rebase before we do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see so better hold off and go ahead with the current state? Or would you still want to see the Toggler::on_change
method in that PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should wait for iced to be rebased before making new changes to iced
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense. Would you suggest blocking this PR and dependencies then? Appreciate there currently seems to be a blocker on iced rebase, so perhaps this change could go in as is, since it included minimal requirement for toggle matching an acceptable UX for async operation.
Alternatively, thinking about the Bluetooth screen, I could put something like the text("...")
with the toggler is not available.
Appreciate all options are sub-optimal here, but just keen to get the ball rolling for COSMIC :)
65f4422
to
2b7d171
Compare
Depends on #pop-os/iced#169